London | 26-ITP-May | Raihan Sharif | Sprint 1 | Destructuring exercises#492
London | 26-ITP-May | Raihan Sharif | Sprint 1 | Destructuring exercises#492RaihanSharif wants to merge 8 commits into
Conversation
RaihanSharif
added
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This all works, but a few questions/suggestions about some details :)
| // Don't change anything else. | ||
| function introduceYourself(___________________________) { | ||
| function introduceYourself({ | ||
| name = "NO NAME", |
There was a problem hiding this comment.
Why did you decide to add default values here?
There was a problem hiding this comment.
I wanted users to be get some feedback if some values were not supplied, rather than just leaving it blank.
I have removed it.
| }, | ||
| ]; | ||
|
|
||
| function logPeopleByHouse(peopleList, houseName) { |
There was a problem hiding this comment.
These solutions work, but don't use destructuring as much as they could. Given this exercises is about destructuring, can you aim for maximum destructring?
There was a problem hiding this comment.
I have added more destructuring, can this be considered maximum descructuring?
I prefer the previous solution in terms of readability. Though I guess if you have a lot of properties it can get tedious.
What do you think?
| order.forEach((item) => { | ||
| const { quantity, itemName, unitPricePence } = item; | ||
| console.log( | ||
| `${String(quantity).padEnd(8)}${String(itemName).padEnd(20)}${((quantity * unitPricePence) / 100).toFixed(2)}` |
There was a problem hiding this comment.
There's quite a lot of repetition with your formatting. Imagine if we start to start padding QTY by only 4 or 12 characters - you'd need to update two different places. Can you work out how to reduce that repetition?
There was a problem hiding this comment.
I thought of adding a reciept formatter or string building function that would take quantity, itemName, and unitPricePence, and return a formatted String for console to output. I thought this might be overkill for a simple receipt function.
Instead I just store the padding values up top as variables, so that padding can be easily adjusted, without having to touch the console log code.
| console.log(`${"QTY".padEnd(8)}${"ITEM".padEnd(20)}TOTAL`); | ||
| let total = 0; | ||
| order.forEach((item) => { | ||
| const { quantity, itemName, unitPricePence } = item; |
There was a problem hiding this comment.
It may be tidier to destructure in the parameter, rather than having a named parameter and them immediately destructuring it. WDYT?
There was a problem hiding this comment.
I agree, just doing the destructuring in the parameter makes it easier to read and understand. Code is now changed.
illicitonion
added
Reviewed
… more dynamic. Don't have to edit the string, just update the padding variables.
RaihanSharif
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
Did all required tasks. In exercise 2, ignore the last function if you just want a minimal working solution.